Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Claude finished @tac0turtle's task in 3m 44s —— View job

Code Review: Amsterdam Fork Prep

• Gather context and understand PR scope
• Review execution/evm/engine_payload.go (new file)
• Review execution/evm/engine_rpc_client.go (major changes)
• Review execution/evm/execution.go
• Review execution/evm/engine_rpc_tracing.go
• Review test files
• Review documentation changes
• Synthesize findings

Overview

This PR adds Amsterdam Engine API support (V4/V5/V6 method negotiation, slotNumber in payload attributes, and blockAccessList passthrough). The overall design is sound — using unsupported-fork fallback for version auto-detection and preserving raw JSON to avoid field loss are good choices. Coverage is strong. A few issues worth addressing before merge:

Bug: Missing json:"blobsBundle" tag on BlobsBundle fields

execution/evm/engine_payload.go:29 and :72

Both the unmarshal and marshal anonymous structs omit the JSON tag for BlobsBundle:

// in both the unmarshal and marshal structs:
BlobsBundle      *engine.BlobsBundle          // missing json tag

Go's JSON encoder will emit this as "BlobsBundle" (capital B), while the Engine API expects "blobsBundle". On decode, Go's case-insensitive matching accepts "blobsBundle", so unmarshaling still works — but MarshalJSON would produce a non-spec-compliant envelope. Should be:

BlobsBundle *engine.BlobsBundle `json:"blobsBundle"`

Fix this →

Bug: NewPayload has no unsupported-fork fallback

execution/evm/engine_rpc_client.go:143-155

GetPayload and ForkchoiceUpdated both retry with fallback versions on -38005. NewPayload silects the method via newPayloadMethod(payload) before the call and never retries:

func (e *engineRPCClient) NewPayload(...) (*engine.PayloadStatusV1, error) {
    ...
    err = e.client.CallContext(ctx, &result, newPayloadMethod(payload), ...)
    if err != nil {
        return nil, err  // no fallback
    }

If engine_newPayloadV5 gets a -38005 response (which shouldn't happen when getPayloadV6 already succeeded, but could happen on inconsistent EL state), the caller sees an opaque error with no recovery path. A fallback to V4 for non-Amsterdam payloads, or at minimum a isUnsupportedForkErr check with an informative error, would match the pattern used elsewhere and make failures easier to diagnose.

Performance: rawExecutionPayloadHasField allocates on hot path

execution/evm/engine_payload.go:141-150

func rawExecutionPayloadHasField(payload json.RawMessage, field string) bool {
    var fields map[string]json.RawMessage
    if err := json.Unmarshal(payload, &fields); err != nil {
        return false
    }
    _, ok := fields[field]
    return ok
}

This is called by hasAmsterdamFields(), which is called in newPayloadMethod() on every NewPayload. Unmarshaling the entire execution payload (which includes the full transaction list) into a map[string]json.RawMessage just to check for a key allocates needlessly on the hot path.

The typed path (e.ExecutionPayload.SlotNumber != nil) is checked first, so this only runs when SlotNumber is nil. For a valid Amsterdam payload that came through getPayloadV6, engine.ExecutableData.SlotNumber should be populated by go-ethereum's decoder, avoiding the raw scan. But a simple bytes.Contains check on the raw JSON (e.g. for "slotNumber") would be faster and allocation-free for the fallback:

// Faster alternative — no full unmarshal needed:
func rawExecutionPayloadHasField(payload json.RawMessage, field string) bool {
    return bytes.Contains(payload, []byte(`"`+field+`"`))
}

(A regex or bytes.Index search is not precise for all JSON structures, but for a known field name it is sufficient in practice.)

Code clarity: RawExecutionPayload copy idiom

execution/evm/engine_payload.go:48

e.RawExecutionPayload = append(e.RawExecutionPayload[:0], dec.ExecutionPayload...)

This is a defensive copy (avoids sharing the underlying byte slice with the decoded temp struct), but the intent is not obvious. Since e.RawExecutionPayload is nil on entry, nil[:0] is valid but unusual. Prefer:

e.RawExecutionPayload = append([]byte(nil), dec.ExecutionPayload...)

or simply:

e.RawExecutionPayload = json.RawMessage(append([]byte(nil), dec.ExecutionPayload...))

Design note: forkchoiceArgsForMethod returns error for V4 without slotNumber

execution/evm/engine_rpc_client.go:218-221

case forkchoiceUpdatedV4Method:
    if _, ok := methodArgs["slotNumber"]; !ok {
        return nil, fmt.Errorf("%s requires slotNumber payload attribute", method)
    }

This returns an error (not a fallback) when non-nil args lack slotNumber for V4. The fallback loop's early return on error means V3 is never tried in this case. Currently safe because:

• All non-nil args come from buildPayloadAttributes, which always includes slotNumber.
• nil args (from setHead/setFinal/etc.) bypass this check.

A comment would make this implicit contract explicit to future readers:

// Note: callers that pass non-nil args must include "slotNumber" once V4 is active.
// Internal calls with nil args (forkchoice-only, no payload build) bypass this check.

Test: strings.Replace for Amsterdam JSON is fragile

execution/evm/engine_rpc_client_test.go:107-119

func minimalAmsterdamPayloadEnvelopeJSON(t *testing.T) string {
    withAmsterdamFields := strings.Replace(
        minimalPayloadEnvelopeJSON,
        `"excessBlobGas": "0x0"`,
        `"excessBlobGas": "0x0",\n\t\t"slotNumber": "0x1",\n\t\t"blockAccessList": ["0x1234"]`,
        1,
    )

This relies on the exact whitespace/formatting of minimalPayloadEnvelopeJSON. The require.NotEqual guard catches silent no-ops, but a direct JSON-merge approach using json.Unmarshal + field assignment would be more maintainable.

Minor: decodeSecret / getAuthToken duplicated in test package

execution/evm/test/test_helpers.go:206-227

These functions are exact copies of the unexported helpers in execution.go. They can't be imported directly since test_helpers.go is in a separate package. Consider exporting or moving shared JWT utilities to a shared internal package to avoid the duplication.

Minor: ams_update.md should not be committed to the repo

ams_update.md is a planning/design document describing work that has now been completed. It should not be part of the committed codebase — it belongs in a PR description, a notion/wiki page, or the repo docs directory as a proper architecture note. Before merging, either delete this file or convert it to docs/architecture/amsterdam-engine-api.md.

Positive observations

• The raw-JSON passthrough approach for blockAccessList in EnginePayloadEnvelope is exactly the right design — it avoids dropping unknown Amsterdam payload fields without requiring a parallel struct definition.
• The fallback order logic ([preferred, next, fallback]) correctly handles both upgrade and downgrade paths.
• When FCU V4 succeeds, eagerly setting payloadVersion to V6 is a nice optimization that avoids the probe-then-fail cycle for getPayload.
• Unit tests in engine_rpc_client_test.go cover all version selection paths well, including the important blockAccessList preservation assertion.
• The E2E test via engineRPCRecorder (proxy + call recorder) is a clean pattern for verifying method version selection against a real execution client.
• Tracing updates correctly reflect the resolved method version after the call (post-call attribute setting), which is the right approach since version is only known after the fallback resolves.

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.71%. Comparing base (29e8542) to head (e7df142).
⚠️ Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3352   +/-   ##
=======================================
  Coverage   60.70%   60.71%           
=======================================
  Files         127      127           
  Lines       13781    13781           
=======================================
+ Hits         8366     8367    +1     
+ Misses       4502     4501    -1     
  Partials      913      913           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.